Improve Generator Logging #2964
Conversation
theunrepentantgeek
requested review from
davefellows,
matthchr,
babbageclunk and
super-harsh
as code owners
Codecov Report
@@ Coverage Diff @@
## main #2964 +/- ##
==========================================
- Coverage 54.20% 54.19% -0.02%
==========================================
Files 1374 1374
Lines 580648 580781 +133
==========================================
+ Hits 314753 314765 +12
- Misses 214109 214244 +135
+ Partials 51786 51772 -14
|
Not entirely sure I am parsing this correctly. Are you saying we have places where we OTOH I'm not all that concerned about places where we panic but we actually want that error to be fatal. I mean we could change those instances, sure - but if it's difficult/painful to do so I am not sure what value we're getting by doing it as at the end of the day the error is going to result in process termination anyway? |
We have places where we panic because we're encountering a situation that our code doesn't handle (due to the way Swagger has been used) rather than because our program has done something unexpected internally. If someone is trying to configure a new resource, it's possible for our generator to simply blow up - and the user would have to try and troubleshoot the problem based purely on the stack trace. When this happens to us, we run the generator under a debugger and can easily work out the problem. We have that option because we're intimately familiar with the code. I don't expect that contributors would necessarily take the time to try and troubleshoot the generator, nor do I think that we should be expecting them to do so. What I want to do is to address these panics by turning them into errors that provide reasonable information and context - then when something goes wrong, users have the information they need to try and resolve the problem, or at least they have good information to put into an issue so we can sort it out. Plus, a program that blows up (for any reason) is likely to be perceived as low quality. One that fails with a detailed error message is better. |
v2/tools/generator/internal/codegen/pipeline/convert_allof_and_oneof_to_objects.go
Outdated
Show resolved
Hide resolved
v2/tools/generator/internal/codegen/pipeline/recursivetypefixer/simple_recursive_type_fixer.go
Show resolved
Hide resolved
…azure-service-operator into improve/generator-logging
| @@ -111,8 +110,6 @@ func NewAzureResourceType(specType Type, statusType Type, typeName TypeName, sco | |||
| } | |||
|
|
|||
| if nameProperty == nil { | |||
| klog.V(1).Infof("resource %s is missing field 'Name', fabricating one...", typeName) | |||
There was a problem hiding this comment.
We can still keep this and other few with a higher level of logging?
There was a problem hiding this comment.
Not sure what you mean. "Higher" as in more important (lower numbers) or "Higher" as in larger number?
ZeroLogr essentially only uses 0 (info), 1 (debug) and 2 (trace).
There was a problem hiding this comment.
yeah my bad, I mean we can retain some of these logs as lower-level logging.
v2/tools/generator/internal/codegen/pipeline/inject_property_assignment_functions.go
Show resolved
Hide resolved
What this PR does / why we need it:
Improves the logging output from our code generator so it's a little less intimidating for users.
Primary change is to switch to ZeroLog. Given this is a commandline tool, the built in overhead of klog seems misplaced.
Closes #2853
We had a bunch of logging that I felt was no longer required - stuff that was likely useful while features were written and bedded in, but which were now unused and uninformative.
I've changed some rare warnings that seem to never happen into panic calls, so that we're aware of them when they do occur (obscure warnings are easy to lose in our current output).
If a panic happens during the execution of a pipeline stage, that's now caught and gracefully handled.
Much of the long period at the start (while loading OpenAPI documents) now has periodic logging so the user knows something is happening, instead of the app sitting there silently for 30-60s.
Also addresses some method deprecations and updates some dependencies.
Special notes for your reviewer:
We've got some technical debt lurking under the hood - places where things can go wrong and we currently panic, but where those issues are expected, and we really should just be returning an error. (And in this PR I've made some of that debt a little worse.) I tried to address it, but it all got out of hand, due largely to the viral nature of
errorreturns in Go. I have a handle on breaking that down into smaller pieces of work and will create issues to track those as we clean things up.How does this PR make you feel:
